-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 4371 incorrect dependencies when install dev packages #5234
Issue 4371 incorrect dependencies when install dev packages #5234
Conversation
pipenv/core.py
Outdated
project, | ||
requirements_dir=requirements_dir, | ||
) | ||
pip_command.extend(["-c", vistir.path.normalize_path(constraint_filename)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an alternative function that would make sense other than calling something from vistir? Also, is there a way to do this without writing to a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing.
There are similar function to normalize path inside shell.py, I will replace vistir with this.
Lines 98 to 101 in 6c0c451
def normalize_path(path): | |
return os.path.expandvars( | |
os.path.expanduser(os.path.normcase(os.path.normpath(os.path.abspath(str(path))))) | |
) |
After reading constraints files, I think currently there are no way to pass constraint as argument to pip.
-
The constraint file created from
write_to_constrain_file
is passed to subprocess, so it might be hard to not use the file.
Lines 1609 to 1610 in 30a6b1a
c = subprocess_run(pip_command, block=block, env=pip_config) c.env = pip_config -
The constraints files create from
default_constraint_file
inside resolver could be removed I think. But this might take time reading pip code base to see how constraints files is convert into constraint line.
pipenv/core.py
Outdated
for k, v in project.packages.items() | ||
if not is_editable(k) and not is_editable(v) and not has_extras(v) | ||
} | ||
constraints = convert_deps_to_pip(constraints, project=project, r=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convert_deps_to_pip
calls quite a bit of code in requirementslib
. I've been studying this code base for 10 months now and I still haven't wrapped my head around all of the resolver/dependency logic, but from spending some time in the debugger last night I know that this method calls a lot of code that we need help unraveling in requirementslib. I am concerned that this PR may be adding to the amount of logic we have to understand around this, and could possibly be duplicating some code that we should be refactoring elsewhere. I'll definitely need more time to study this change, but I am still glad you are pushing forward with a solution to the original issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the duplicated code I mentioned is actually myself.
Lines 1462 to 1470 in 30a6b1a
# duplicated in pipenv.utils.resolver | |
# https://pip.pypa.io/en/stable/user_guide/#constraints-files | |
# constraints must have a name, they cannot be editable, and they cannot specify extras. | |
constraints = { | |
k: v | |
for k, v in project.packages.items() | |
if not is_editable(k) and not is_editable(v) and not has_extras(v) | |
} | |
constraints = convert_deps_to_pip(constraints, project=project, r=False) |
pipenv/pipenv/utils/resolver.py
Lines 565 to 574 in 30a6b1a
def get_default_constraints_from_packages(): | |
# https://pip.pypa.io/en/stable/user_guide/#constraints-files | |
# constraints must have a name, they cannot be editable, and they cannot specify extras. | |
constraints = { | |
k: v | |
for k, v in self.project.packages.items() | |
if not is_editable(k) and not is_editable(v) and not has_extras(v) | |
} | |
constraints = convert_deps_to_pip(constraints, project=self.project, r=False) | |
return constraints |
I'm not sure should I write an attribute which generate constraints packages inside class Project or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest extracting it from resolver.py to a standalone util, that doesn't depend on a class and re-use it in both places. Instead of self.project
it can just take a project as a parameter. I am not sure if it makes more sense to live in "resolver.py" or "dependencies.py" but dependencies.py already has the methods convert_deps_to_pip
and is_editable
so I am leaning towards putting it there, plus its a smaller file than resolver.py which is quite large still--would be good to chip away at the code organization improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, it seems reasonable to have it inside "dependencies.py". I updated the code.
tests/integration/test_lock.py
Outdated
@@ -791,3 +791,39 @@ def test_pipenv_respects_package_index_restrictions(PipenvInstance): | |||
'sha256:ec22d826a36ed72a7358ff3fe56cbd4ba69dd7a6718ffd450ff0e9df7a47ce6a'], | |||
'index': 'local', 'version': '==2.19.1'} | |||
assert p.lockfile['default']['requests'] == expected_result | |||
|
|||
|
|||
@flaky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be adding any new flaky tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove it. Frankly, I don't know what it does, I use it because I saw other tests functions use it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was considered for tests that would sometimes fail to have them re-run, but we expect our tests to pass so its just an odd annotation.
@pytest.mark.dev | ||
@pytest.mark.lock | ||
@pytest.mark.install | ||
def test_dev_lock_use_default_packages_as_constraint(PipenvInstance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this test in the debugger, it gets to the method pip_install
but use_constraint=False
and so it does not exercise the method write_constraint_to_file
as I would have thought. Going to try with your other tests to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there are 2 constraints files. The first one is inside resolver.py
pipenv/pipenv/utils/resolver.py
Lines 591 to 595 in 30a6b1a
@property | |
def default_constraint_file(self): | |
if self._default_constraint_file is None: | |
self._default_constraint_file = self.prepare_default_constraint_file() | |
return self._default_constraint_file |
The second one is
write_constraint_to_file
as mention above.
- The first constraints file is created and used when: Locking or Installing with Pipfile using
--dev
flag (user don't pass any packages args). - The second constrains file is created and used when: User pass packages as argument using
--dev
flag.
When locking, the code does the following things:
- Resolve dependencies with
venv_resolve_deps
(this create and use the first constraints file). - Running
batch_install
, install all packages in the lock file. This one callspip_install
, this could create the second constraints file (if bothdev
anduse_constraint
equal True). But since all the packages were resolved in step 1 and written to lockfile, so resolving isn't necessary.
Lines 785 to 793 in 30a6b1a
no_deps=skip_dependencies, block=is_blocking, index=dep.index, requirements_dir=requirements_dir, pypi_mirror=pypi_mirror, trusted_hosts=trusted_hosts, extra_indexes=extra_indexes, use_pep517=use_pep517, use_constraint=False, # no need to use constraints, it's written in lockfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dqkqd -- I was able to hit that breakpoint in the test test_install_dev_use_default_constraints
after I posted the first message. Can you explain more why to have two different methods of similar code for generating the two different constraints files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have lock
and install
method. The first constraint file in resolver.py
is used for lock
, the second one in core.py
is used for install
.
- The first one should not be removed, because it would cause lockfile is not generated correctly.
- The second one could be remove. Actually, this is my first approach.
Suppose I remove this one. Thenpip_install
inside this loop should not be called, because it will install packages without resolving dependencies.
Lines 2136 to 2271 in 30a6b1a
for pkg_line in pkg_list: click.secho( fix_utf8(f"Installing {pkg_line}..."), fg="green", bold=True, ) # pip install: with vistir.contextmanagers.temp_environ(), create_spinner( "Installing...", project.s ) as sp: if not system: os.environ["PIP_USER"] = "0" if "PYTHONHOME" in os.environ: del os.environ["PYTHONHOME"] sp.text = f"Resolving {pkg_line}..." try: pkg_requirement = Requirement.from_line(pkg_line) except ValueError as e: sp.write_err("{}: {}".format(click.style("WARNING", fg="red"), e)) sp.red.fail( environments.PIPENV_SPINNER_FAIL_TEXT.format( "Installation Failed" ) ) sys.exit(1) sp.text = "Installing..." try: sp.text = f"Installing {pkg_requirement.name}..." if project.s.is_verbose(): sp.hide_and_write( f"Installing package: {pkg_requirement.as_line(include_hashes=False)}" ) c = pip_install( project, pkg_requirement, ignore_hashes=True, allow_global=system, selective_upgrade=selective_upgrade, no_deps=False, pre=pre, dev=dev, requirements_dir=requirements_directory, index=index_url, pypi_mirror=pypi_mirror, use_constraint=True, ) if c.returncode: sp.write_err( "{} An error occurred while installing {}!".format( click.style("Error: ", fg="red", bold=True), click.style(pkg_line, fg="green"), ), ) sp.write_err(f"Error text: {c.stdout}") sp.write_err(click.style(format_pip_error(c.stderr), fg="cyan")) if project.s.is_verbose(): sp.write_err( click.style(format_pip_output(c.stdout), fg="cyan") ) if "setup.py egg_info" in c.stderr: sp.write_err( "This is likely caused by a bug in {}. " "Report this to its maintainers.".format( click.style(pkg_requirement.name, fg="green") ) ) sp.red.fail( environments.PIPENV_SPINNER_FAIL_TEXT.format( "Installation Failed" ) ) sys.exit(1) except (ValueError, RuntimeError) as e: sp.write_err("{}: {}".format(click.style("WARNING", fg="red"), e)) sp.red.fail( environments.PIPENV_SPINNER_FAIL_TEXT.format( "Installation Failed", ) ) sys.exit(1) # Warn if --editable wasn't passed. if ( pkg_requirement.is_vcs and not pkg_requirement.editable and not project.s.PIPENV_RESOLVE_VCS ): sp.write_err( "{}: You installed a VCS dependency in non-editable mode. " "This will work fine, but sub-dependencies will not be resolved by {}." "\n To enable this sub-dependency functionality, specify that this dependency is editable." "".format( click.style("Warning", fg="red", bold=True), click.style("$ pipenv lock", fg="yellow"), ) ) sp.write( "{} {} {} {}{}".format( click.style("Adding", bold=True), click.style(f"{pkg_requirement.name}", fg="green", bold=True), click.style("to Pipfile's", bold=True), click.style( "[dev-packages]" if dev else "[packages]", fg="yellow", bold=True, ), click.style(fix_utf8("..."), bold=True), ) ) # Add the package to the Pipfile. if index_url: index_name = project.add_index_to_pipfile( index_url, verify_ssl=index_url.startswith("https:") ) pkg_requirement.index = index_name try: project.add_package_to_pipfile(pkg_requirement, dev) except ValueError: import traceback sp.write_err( "{} {}".format( click.style("Error:", fg="red", bold=True), traceback.format_exc(), ) ) sp.fail( environments.PIPENV_SPINNER_FAIL_TEXT.format( "Failed adding package to Pipfile" ) ) sp.ok( environments.PIPENV_SPINNER_OK_TEXT.format("Installation Succeeded") ) # Update project settings with pre preference. if pre: project.update_settings({"allow_prereleases": pre})
Sopip_install
should be replaced by anothercheck_installable_package
method. After that, resolving all the packages withdo_lock
(which use the first constraints file) will give us the same result.
I didn't use this approach because:$ pip search
is not working, I could not find a way to check if a package is installable without calling$ pip install
- The loop locks scary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there is another case where resolving dependencies inside pip_install
is necessary.
For example, I have packages installed with this Pipfile:
[packages]
django = "<3.0"
Suppose I install new dev package using command $ pipenv install --dev "django>3.0,<4.0"
. Clearly the package is installable but it shouldn't be, because it would conflict with default packages.
pipenv/utils/resolver.py
Outdated
default_constraints_file.close() | ||
return default_constraints_file.name | ||
|
||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a @cached_property
instead so that we don't have to implement that ourselves with if self._default_constraint_file is None:
logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used @cached_property
but falling some tests on MacOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those tests also failed without @cached_property
as well, so maybe @cached_property
is not the issue. Because @cached_property
does not work with python 3.7, I used @property
and @lru_cache
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is you need to import @cached_property
from the vendor'd dependencies, go with from pipenv.vendor.cached_property import cached_property
until #5169 is completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only tests that are expected could fail, and likely on mac OS right now is test_convert_deps_to_pip
and test_convert_deps_to_pip_one_way
. Still investigating how these failures started in the main branch, so if you see that specifically its safe to ignore. The other tests should all pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran with the main branch, but there are more failed tests on macOS other than test_convert_deps_to_pip
and test_convert_deps_to_pip_one_way
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dqkqd It is possible that the new version of setuptools that was released yesterday has affected things. I will re-run the main branch actions, but you can see the last time it ran upon merge ony those *_depts_to_pip
tests had failed: https://github.com/pypa/pipenv/runs/7758310446?check_suite_focus=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matteius I updated as you said. Not sure why it passed *_depts_to_pip
cases. I didn't even modify convert_deps_to_pip
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dqkqd It is a "flakey" test case, maybe I'll mark it as such and it won't fail the build as often. I haven't gotten to the bottom of why sometimes it behaves differently, it has failed on Mac OS most frequently, but I have seen it once in a while fail on other systems.
pipenv/utils/resolver.py
Outdated
@@ -630,6 +660,37 @@ def parsed_constraints(self): | |||
) | |||
return self._parsed_constraints | |||
|
|||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend also making this a @cached_property
pipenv/utils/resolver.py
Outdated
) | ||
return self._parsed_default_constraints | ||
|
||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one also I think would be better as a cached_property.
pipenv/utils/resolver.py
Outdated
|
||
@property | ||
def default_constraints(self): | ||
from pipenv.patched.pip._internal.req.constructors import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better if these imports were at the top of the files -- shouldn't be a circular import in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that at the top of the file.
@@ -646,6 +707,8 @@ def constraints(self): | |||
) | |||
for c in self.parsed_constraints | |||
] | |||
if self.dev: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain more why we only include the default_constraints when dev is specified? I don't think I quite understand out of the gate. I got to looking at this more because this is the only place that uses self.dev, and I am noticing how many parameters pip_install
takes already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think constraint is used only when installing dev-packages. If it does with new normal packages, the installing package could not overwrite existing packages, because it use them as contraint, and therefore might not be installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok thanks, that makes sense.
pipenv/utils/resolver.py
Outdated
@@ -552,6 +560,28 @@ def constraint_file(self): | |||
self._constraint_file = self.prepare_constraint_file() | |||
return self._constraint_file | |||
|
|||
def prepare_default_constraint_file(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like something that could be a standalone utility in dependencies.py that is not method of the class that might be helpful towards future refactors. something like def prepare_default_constraint_file(directory, project)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, on second thought, this feels very similar to the method get_constraints_from_deps
-- Perhaps we could extract out the creation of the tracked tempfile and the writing to it of the constraints out to a shared method that takes the directory and the constraints to be written. Like def prepare_default_constraint_file(directory, constraints)
. It could be re-used by write_constraint_to_file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I will try this. Btw, I might not be able to response to this issue in the next few days. I’m sorry. Will get back on to this as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could also be re-used by prepare_constraint_file
which has similar logic right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote standalone utility in dependencies.py, but not sure how to use it in prepare_constraint_file
. Maybe my implementation is not good. Please tell me if there are any room for improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dqkqd Thanks, it looks great, basically I think you need to add a third parameter which is the constraints
and have that computed outside the helper utility file, because that is what is different about the three places that write this file. I think if we can solve for that, it will be ready for merge :-D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh -- and please add a news fragment :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified it and add news fragment.
@dqkqd I left a few more notes about areas I think we could further refine the code and reduce duplication. I think if we can factor out the three spots we write constraint files into a common utility and some of the places we can use |
pipenv/utils/dependencies.py
Outdated
@@ -298,6 +298,31 @@ def is_constraint(dep): | |||
return constraints | |||
|
|||
|
|||
def prepare_default_constraint_file( | |||
project, | |||
dir=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid shadowing built in python function name dir
-- I recommend naming it directory
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it.
@dqkqd Nice work! |
@matteius Thank you. |
* Add test, ensure dev lock use default packages as constraints. * Use default packages as constraints when locking develop packages. * Add test, ensure installing dev-packages use default packages as constraints. (pypa#4371) (pypa#2987) * Use default packages as constraints when installing provided dev packages. * change vistir.path.normalize_path to pipenv.utils.shell.normalize_path * Add function that get contraints from packages. * Add test for get_constraints_from_deps function * Use get_constraints_from_deps to get constraints * Use @cached_property instead of @Property * Use standalone utility to write constraints file * prepare_constraint_file use precomputed constraints. * Add news fragment.
The issue
I'm trying to fix #4371. I think it's better to consider 2 cases below:
The fix
Installing from Pipfile
I read through the code, and here is what it does when installing packages from Pipfile:
dev-packages
dependencies. Write to lockfile as default.packages
dependencies. Write to lockfile as develop.Because,
dev-packages
is resolved separately, there might be a chance wheredev-packages
is overwritten but its dependencies are not (they aren't duplicated withpackages
). So I think the result dependency graph would be more accurate ifpackages
is considered as constraint while resolvingdev-packages
dependencies. Luckily pip provided constraints files.I modified the code to do the followings:
packages
dependencies. Write to lockfile.packages
. Skip editable and extras packages as describe in pip constraints files.dev-packages
dependencies using constraints file above. Write to lockfile as default.dev-packages
was resolved with constraints, this step is also important because there some cases constraints might be skipped (editable and extras packages).Installing from provided packages
When packages are passed as arguments, pipenv does the followings:
dev-packages
andpackages
independently. Modify the lockfile.Similar to the first problem, I tried to use default packages as constraints to resolve packages dependencies. I think it is enough to just resolve
dev-packages
at step 1. Because default packages should be installed directly, and resolved packages at step 2 don't need to be resolved again.The checklist
news/
directory to describe this fix with the extension.bugfix
,.feature
,.behavior
,.doc
..vendor
. or.trivial
(this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.